fix: protect internal session variables against user overwrite#10294
fix: protect internal session variables against user overwrite#10294gr8man wants to merge 6 commits into
Conversation
michalsn
left a comment
michalsn
left a comment
There was a problem hiding this comment.
In this scenario, which should be very unlikely, overwriting __ci_* values would probably be the smallest problem. At that point, the application would already have allowed users to control session keys. Depending on how the application uses session data, this could also include application-level keys such as user_id, role, is_admin, etc.
Not sure what others will say, but if we want to restrict these, then I would target the 4.8 branch. Besides skipping, we should also log a warning so the skipped values are not silently ignored.
This would still be backward-compatible in intent, since applications should not use framework-reserved __ci_* keys, but it is not strictly behavior-compatible because those keys are currently accepted by the public session API.
Co-authored-by: Michal Sniatala <michal@sniatala.pl>
|
You make a very good point. I agree that the actual security benefit here is minimal, since an attacker who can inject arbitrary keys into the session can already do enough damage by overriding application-level keys (like user_id or is_admin). Given that this also introduces a slight behavior change (BC break) by silently ignoring or restricting keys that the public API previously accepted, and would require targeting the 4.8 branch, I think it's better to just close this PR. It's not worth the added complexity for such a small robustness gain. Thanks for the feedback and the review! |
2 participants
Description
Currently, the
Sessionclass lacks protection against overwriting internal framework session keys. If a developer sets session values directly from user input (e.g., using$session->set($this->request->getPost())), a malicious user can inject the__ci_varsor__ci_last_regenerateparameters.Depending on the injected payload, this can lead to:
__ci_varsis set to a string, the next session initialization will throw aTypeErrorwheninitVars()attempts to iterate over it withforeach. This effectively breaks the session and locks the user out until the session expires or cookies are cleared.__ci_varsis crafted as an array, it can trick the garbage collection ininitVars()intounset()-ting arbitrary session keys within the user's session.This PR adds a straightforward validation rule inside the
set()and__set()methods to silently ignore setting any keys that start with the framework's internal prefix__ci_.Note: The built-in methods for handling flashdata and tempdata (like
markAsFlashdata) modify$_SESSION['__ci_vars']directly as superglobals and are completely unaffected by this change. It ensures full backwards compatibility.Checklist: